Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEV: _get_images can take header or list of headers as input #61

Merged
merged 3 commits into from
Apr 3, 2018

Conversation

licode
Copy link
Contributor

@licode licode commented Mar 30, 2018

This is to be compatible with previous api that _get_images can take list of headers as input.

@coveralls
Copy link

coveralls commented Mar 30, 2018

Coverage Status

Coverage decreased (-0.2%) to 28.723% when pulling 8a4e8ca on licode:multi_headers into 3727870 on NSLS-II-CSX:master.

@codecov-io
Copy link

codecov-io commented Mar 30, 2018

Codecov Report

Merging #61 into master will decrease coverage by 0.2%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
- Coverage   28.92%   28.72%   -0.21%     
==========================================
  Files          10       10              
  Lines         280      282       +2     
==========================================
  Hits           81       81              
- Misses        199      201       +2
Impacted Files Coverage Δ
csxtools/utils.py 20% <0%> (-0.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3727870...8a4e8ca. Read the comment docs.

@@ -178,7 +178,10 @@ def get_images_to_3D(images, dtype=None):

def _get_images(header, tag, roi=None):
t = ttime.time()
images = header.db.get_images(header, tag)
if isinstance(header, (list, tuple)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this to use header.data instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for suggestion on new api. updated

@@ -178,7 +178,10 @@ def get_images_to_3D(images, dtype=None):

def _get_images(header, tag, roi=None):
t = ttime.time()
images = header.db.get_images(header, tag)
if isinstance(header, (list, tuple)):
images = header[0].data(tag)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we only interested in the first image here?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrakitin is correct -- we don't want to throw away headers in the case of a list input.

We want to be able to use two (or more generally an arbitrary list of) gain 8 dark images to correct the data and this functionality was broken in the last update.

from databroker import DataBroker as db
from csxtools.utils import get_fastccd_images

old_scan_no = 85528
slicerator = get_fastccd_images(db[old_scan_no], (db[old_scan_no-1, old_scan_no+1], None, None))

i.e. 85527 & 85529 are dark scan_ids with gain 8.

Copy link
Member

@mrakitin mrakitin Apr 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can do something like this:

images = []
if isinstance(header, (list, tuple)):
    for i in range(len(header)):
        images.append(header[i].data(tag))

I'm not sure if there needs to be a corresponding update downstream.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected the code above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(h.data(tag) for h in header) is not working. It fails at get_images_to_3D function. We need a pims object.

@tacaswell
Copy link
Member

@mpmdean What is the expected signature of this function?

@mpmdean
Copy link

mpmdean commented Apr 2, 2018

@tacaswell

Below is the error from the latest CSX kernal on srv1. It's trying to access the _get_images method on the list of two headers. This works in older kernals.

AttributeError Traceback (most recent call last)
in ()
3
4 old_scan_no = 85528
----> 5 slicerator = get_fastccd_images(db[old_scan_no], (db[old_scan_no-1, old_scan_no+1], None, None))

/opt/conda_envs/analysis-2018-1.0/lib/python3.6/site-packages/csxtools/utils.py in get_fastccd_images(light_header, dark_headers, flat, gain, tag, roi)
85 # Get the images
86
---> 87 bgnd_events = _get_images(d, tag, roi)
88
89 # We assume that all images are for the background

/opt/conda_envs/analysis-2018-1.0/lib/python3.6/site-packages/csxtools/utils.py in _get_images(header, tag, roi)
179 def _get_images(header, tag, roi=None):
180 t = ttime.time()
--> 181 images = header.db.get_images(header, tag)
182 t = ttime.time() - t
183 logger.info("Took %.3f seconds to read data using get_images", t)

AttributeError: 'list' object has no attribute 'db'

I believe this comes from this change:

-def _get_images(header, tag, roi=None, handler_override=None):
+def _get_images(header, tag, roi=None):
t = ttime.time()

images = get_images(header, tag, handler_override=handler_override)
images = header.db.get_images(header, tag)
results in an incompatibility described below. Or at least something that previously worked now does not. The variable header becomes a list if one wants to use.

@licode
Copy link
Contributor Author

licode commented Apr 3, 2018

I think this should work. I also did some local test. any more comment?

Copy link
Member

@mrakitin mrakitin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks @licode

@licode licode merged commit eb3ab7f into NSLS-II-CSX:master Apr 3, 2018
@ambarb
Copy link
Contributor

ambarb commented Jul 23, 2018

Hi All,
I am using the current version of the SRV1 server at CSX (assuming /opt/conda_envs/analysis-2018-2.1). The version of csxtools is '0.1.13'. I am unable to use multiple dark images, but the discussion here indicates that this is the fix. what version of csxtools is supposed to this ability repaired?

@ambarb
Copy link
Contributor

ambarb commented Aug 29, 2018

I am not sure what happened, but now this functionality seems to work. What is very scary is that the version of csxtools remains the same so I don't really understand why I had the issue in the first place. I tried all 3 23id1 servers before and found this issue. But just this week, I forgot and ran other notebooks and it seemed to be fixed. I am going to close the issue on csxtools

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants